Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-120417: Fix "imported but unused" linter warnings #120461

Merged
merged 3 commits into from
Jun 14, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 13, 2024

Add __all__ to the following modules: collections.abc, importlib.machinery, importlib.util and xml.sax.

Add also "# noqa: F401" in subprocess and xml.sax on two lines.

Add __all__ to the following modules: collections.abc,
importlib.machinery, importlib.util and xml.sax.

Add also "# noqa: F401" in subprocess and xml.sax on two lines.
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only nitpick is to put them in alphabetical order, to make it easier to find an import when scanning the list.

@vstinner
Copy link
Member Author

vstinner commented Jun 13, 2024

My only nitpick is to put them in alphabetical order, to make it easier to find an import when scanning the list.

Are you talking about __all__? I put them in definition order. I have no preference.

Lib/collections/abc.py Outdated Show resolved Hide resolved
Lib/importlib/util.py Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

I updated the PR:

  • Remove private names (_ prefix) in __all__
  • Sort __all__
  • Remove collections.abc.__all__: use # noqa instead

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is easy to get __all__ de-synchronized with the module content. I think that all changed modules need tests for __all__. See existing tests that use support.check__all__.

@vstinner
Copy link
Member Author

I think that all changed modules need tests for all. See existing tests that use support.check__all__.

Ok, I added tests.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I tested that __all__ includes all non-underscored names except module names.

@vstinner vstinner merged commit 05df063 into python:main Jun 14, 2024
31 checks passed
@vstinner vstinner deleted the import_qa3 branch June 14, 2024 18:39
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
…0461)

Add __all__ to the following modules:
importlib.machinery, importlib.util and xml.sax.

Add also "# noqa: F401" in collections.abc,
subprocess and xml.sax.

* Sort __all__; remove collections.abc.__all__; remove private names

* Add tests
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
…0461)

Add __all__ to the following modules:
importlib.machinery, importlib.util and xml.sax.

Add also "# noqa: F401" in collections.abc,
subprocess and xml.sax.

* Sort __all__; remove collections.abc.__all__; remove private names

* Add tests
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…0461)

Add __all__ to the following modules:
importlib.machinery, importlib.util and xml.sax.

Add also "# noqa: F401" in collections.abc,
subprocess and xml.sax.

* Sort __all__; remove collections.abc.__all__; remove private names

* Add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants